[video_player] Add new option for allowBackgroundPlayback#5013
[video_player] Add new option for allowBackgroundPlayback#5013fluttergithubbot merged 39 commits intomainfrom unknown repository
Conversation
# Conflicts: # packages/video_player/video_player_platform_interface/CHANGELOG.md
- Add _ambiguate to address an analyzer error - Increase the required version for video_player_platform_interface to 5.1.0, the first version where allowBackgroundPlayback appears
- always_specify_types - avoid_bool_literal_in_conditional_expressions - avoid_function_literal_in_foreach_calls - terminating new line
| video_player_android: ^2.2.17 | ||
| video_player_avfoundation: ^2.2.17 | ||
| video_player_platform_interface: ">=4.2.0 <6.0.0" | ||
| video_player_platform_interface: ">=5.1.0 <6.0.0" |
There was a problem hiding this comment.
Nit: This should just be ^5.1.0. The other syntax was specifically to allow two major versions of the platform interface for a while.
| } | ||
|
|
||
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( |
There was a problem hiding this comment.
Nit: This should be private (_ verifyPlayingWhenAppLifecyclePaused)
| } | ||
|
|
||
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( |
There was a problem hiding this comment.
The name here seems confusing. How about _verifyPlayStateRespondsToLifecycle?
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( | ||
| VideoPlayerController controller, { | ||
| required bool isObserving, |
There was a problem hiding this comment.
s/isObsvering/shouldPauseInBackground/ to make what is being validated (rather than the implementation details of how that is achieved) clearer.
| VideoPlayerController controller, { | ||
| required bool isObserving, | ||
| }) { | ||
| final bool wasPlayingBeforePause = controller.value.isPlaying; |
There was a problem hiding this comment.
It doesn't make sense for a test to call this method when it's not playing already, so I would eliminate this and just start with expect(controller.value.isPlaying, true); as a starting condition. That should make the method easier to understand.
|
@gaaclarke for second review |
|
Thanks for the the review @stuartmorgan! |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I made one minor formatting tweak to pubspec.yaml; LGTM with that.
|
You'll need to merge master in again; there was an authentication change for Firebase Test Lab that required a commit, which is why this last CI run failed. |
| expect(controller.videoPlayerOptions!.mixWithOthers, true); | ||
| }); | ||
|
|
||
| for (final bool allowBackgroundPlayback in <bool>[true, false]) { |
There was a problem hiding this comment.
nit: instead of looping and asserting 2 different things, this should be 2 different independent tests (factor out shared code if needed). It's much easier to grok tests that are asserting absolutes instead of having the extra layer of indirection.
This PR picks up and continues #4908 to support background play
Fixes flutter/flutter#63836